Add opt-in multithreading #85

matthewkeil · 2023-11-05T19:27:04Z

We have a use case for utilizing our level db from multiple worker threads. The base library did most of the heavy lifting and this PR allows the bindings to be used across workers.

Methodology

Context-awareness was preserved and so was threadsafe operation from the node.js perspective. The open and close functionality was all that needed to change and a single mutex protects the setup. A flag was added so the feature is non-breaking and only provides additional functionality.

The open_db function searches for a record in a db_handles map, and if it exists the open_handle_count is incremented for that database. This was used to allow for multiple databases to be accessed per process. The reverse happens when the destructor or the closed_db method is called. If the handle is the last to be closed then the db instance is deleted from the map to cleanup the native LOCK.

vweevers · 2023-11-06T16:04:18Z

Interesting! I didn't realize we were this close to supporting this use case.

What's your reasoning for making this opt-in (through allowMultiThreading)?

matthewkeil · 2023-11-06T16:26:44Z

Interesting! I didn't realize we were this close to supporting this use case.

What's your reasoning for making this opt-in (through allowMultiThreading)?

Mostly so the change was non-breaking for existing users @vweevers. Before I implemented the flag, a few of the unit tests broke so assumed there might be users out there somewhere that are also counting on that behavior. The flag can be stripped out though or I suppose can default to true

matthewkeil · 2023-11-06T16:35:54Z

There is a workflow issue due to python issue on macos.

https://stackoverflow.com/questions/77233855/why-did-i-got-an-error-modulenotfounderror-no-module-named-distutils

Would you like that fixed @vweevers?

Ref nodejs/node-gyp#2869 Ref #85 (comment)

vweevers · 2023-11-06T17:43:43Z

Fixed in main: 3b1a6f2

matthewkeil · 2023-11-06T17:49:29Z

Fixed in main: 3b1a6f2

Would you mind re-running the CI please @vweevers ?

vweevers · 2023-11-06T18:00:17Z

You'll need to rebase your branch to get the fix.

…ened db

matthewkeil · 2023-11-06T18:12:13Z

You'll need to rebase your branch to get the fix.

Thanks for the quick fix @vweevers. Rebased!!

vweevers · 2023-11-06T19:25:59Z

The flag can be stripped out though

Yeah. We can do that in a future semver-major version. It would simplify the code a bit, but I think it's good to introduce this feature gradually.

binding.cc

index.d.ts

binding.cc

README.md

matthewkeil · 2023-11-07T18:34:39Z

@vweevers I think I got all of your comments addressed. Please let me know if there is anything else that you would like updated.

vweevers · 2023-11-07T19:09:25Z

It might be surprising that with multithreading: false a handle is still created. E.g. in this example from the tests:

const location = tempy.directory()
const db1 = new ClassicLevel(location)
await db1.open()
t.is(db1.location, location)

One would expect to have exclusive access to this db location. But the code that follows shows the opposite:

const db2 = new ClassicLevel(location)
await db2.open({ multithreading: true })
t.is(db2.location, location)

In other words, I think multithreading: false should also mean "don't allow others to open this db location". Closer to current behavior.

matthewkeil · 2023-11-08T11:45:41Z

In other words, I think multithreading: false should also mean "don't allow others to open this db location". Closer to current behavior.

Sounds good!! Updated to achieve that behavior @vweevers. I also added another test case to verify the fix

binding.cc

vweevers

Nice work! I'll wait with merging for a few days, to give others a chance to review (@juliangruber @ralphtheninja if you're able 🙏).

matthewkeil · 2023-11-20T10:59:01Z

Nice work! I'll wait with merging for a few days, to give others a chance to review (@juliangruber @ralphtheninja if you're able 🙏).

Hi there @vweevers! Was curious if @juliangruber or @ralphtheninja were interested in taking a bite at the review apple? I will be happy to respond to any comments or concerns.

ralphtheninja · 2023-11-21T13:19:27Z

Aaah! So basically we have a layer inbetween db instances and the corresponding location. Looks good!

matthewkeil · 2023-11-26T18:42:40Z

Aaah! So basically we have a layer inbetween db instances and the corresponding location. Looks good!

Thanks @ralphtheninja! Much appreciated!! @juliangruber do you have any comments or concerns? @vweevers was curious before merging this... Just a nudge so we can get this imported into the chainsafe/lodestar repo

vweevers · 2023-11-26T19:34:04Z

That'll do.

matthewkeil · 2023-11-26T19:40:39Z

@vweevers not sure why this unit test failed on the release...

https://github.com/Level/classic-level/actions/runs/6997605304/job/19034756860

but passed here:
https://github.com/Level/classic-level/actions/runs/6997605338/job/19034756959

UPDATED: looks like there is a race condition. also failed here:
https://github.com/Level/classic-level/actions/runs/6997605338/job/19034757374

vweevers · 2023-11-26T19:46:43Z

Could be a race issue; can you check? I'll wait with the npm release.

For future reference (when the logs are deleted), the error is:

# check multithreading flag works as expected
not ok 5699 Error: Database is not open
  ---
    operator: error
    stack: |-
      Error: Database is not open
          at maybeOpened (/home/runner/work/classic-level/classic-level/node_modules/abstract-level/abstract-level.js:133:18)
          at /home/runner/work/classic-level/classic-level/node_modules/abstract-level/abstract-level.js:160:13
          at process.processTicksAndRejections (node:internal/process/task_queues:78:11)
  ...
not ok 5700 plan != count
  ---
    operator: fail
    expected: 9
    actual:   1
    stack: |-
      Error: plan != count
          at Test.assert [as _assert] (/home/runner/work/classic-level/classic-level/node_modules/tape/lib/test.js:479:48)
          at Test.fail (/home/runner/work/classic-level/classic-level/node_modules/tape/lib/test.js:573:7)
          at completeEnd (/home/runner/work/classic-level/classic-level/node_modules/tape/lib/test.js:394:9)
          at next (/home/runner/work/classic-level/classic-level/node_modules/tape/lib/test.js:404:4)
          at Test._end (/home/runner/work/classic-level/classic-level/node_modules/tape/lib/test.js:424:2)
          at Test.end (/home/runner/work/classic-level/classic-level/node_modules/tape/lib/test.js:198:7)
          at onError (/home/runner/work/classic-level/classic-level/node_modules/tape/lib/test.js:133:10)
          at process.processTicksAndRejections (node:internal/process/task_queues:96:5)
  ...

matthewkeil · 2023-11-26T19:47:24Z

Could be a race issue; can you check? I'll wait with the npm release

Yep. Will take a look this evening

matthewkeil marked this pull request as ready for review November 6, 2023 15:36

vweevers added a commit that referenced this pull request Nov 6, 2023

Fix Python compatibility in CI

3b1a6f2

Ref nodejs/node-gyp#2869 Ref #85 (comment)

matthewkeil added 11 commits November 6, 2023 21:11

feat: threadsafe open and close to allow workers access to already op…

d5eebe4

…ened db

fix: add mutex include

44784df

feat: add allowMultiThreading option

1115e8b

test: add multithreading-test and worker-test and worker-utils

884d1a0

test: clean up unit tests for worker

f243ce9

test: add read/write multithreading case

f8ea407

chore: lint new code

3519bc9

refactor: rename worker-test to worker

db88285

test: add case for passing allowMultiThreading to constructor

3709516

docs: add flag to README

fb1a25c

fix: remove nested conditional

1f4f0c0

matthewkeil force-pushed the multithreading branch from 352a7bc to 1f4f0c0 Compare November 6, 2023 18:11

vweevers reviewed Nov 6, 2023

View reviewed changes

binding.cc Outdated Show resolved Hide resolved

vweevers reviewed Nov 6, 2023

View reviewed changes

index.d.ts Outdated Show resolved Hide resolved

vweevers reviewed Nov 6, 2023

View reviewed changes

binding.cc Outdated Show resolved Hide resolved

vweevers reviewed Nov 6, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

matthewkeil added 3 commits November 7, 2023 09:39

refactor: change allowMultiThreading flag to multithreading

7e41b4b

docs: PR comment docstring changes

4aa3c12

fix: change NotFound to OK status for invalid handle map iterator

f47200b

feat: update multithreading so 'false' removes ability to open multiple

5d4d40e

matthewkeil added 4 commits November 8, 2023 14:46

chore: run lint

8427a70

fix: change test error strings

b60e6ee

fix: change test error strings

8d45bba

fix: change expected error in lock-test

860daf1

vweevers reviewed Nov 8, 2023

View reviewed changes

binding.cc Outdated Show resolved Hide resolved

refactor: update multithreading handling per PR

70da60a

vweevers approved these changes Nov 9, 2023

View reviewed changes

vweevers changed the title ~~Worker Multi-Threading~~ Add opt-in multithreading Nov 26, 2023

vweevers merged commit 7d497a5 into Level:main Nov 26, 2023
15 checks passed

matthewkeil mentioned this pull request Jan 19, 2024

feat: add x64-debug build recipe nodejs/unofficial-builds#107

Closed

vweevers mentioned this pull request Feb 3, 2024

Workers #19

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add opt-in multithreading #85

Add opt-in multithreading #85

matthewkeil commented Nov 5, 2023 •

edited

Loading

vweevers commented Nov 6, 2023

matthewkeil commented Nov 6, 2023

matthewkeil commented Nov 6, 2023

vweevers commented Nov 6, 2023

matthewkeil commented Nov 6, 2023 •

edited

Loading

vweevers commented Nov 6, 2023

matthewkeil commented Nov 6, 2023

vweevers commented Nov 6, 2023

matthewkeil commented Nov 7, 2023

vweevers commented Nov 7, 2023

matthewkeil commented Nov 8, 2023 •

edited

Loading

vweevers left a comment

matthewkeil commented Nov 20, 2023

ralphtheninja commented Nov 21, 2023

matthewkeil commented Nov 26, 2023

vweevers commented Nov 26, 2023

matthewkeil commented Nov 26, 2023 •

edited

Loading

vweevers commented Nov 26, 2023 •

edited

Loading

matthewkeil commented Nov 26, 2023

Add opt-in multithreading #85

Add opt-in multithreading #85

Conversation

matthewkeil commented Nov 5, 2023 • edited Loading

Methodology

vweevers commented Nov 6, 2023

matthewkeil commented Nov 6, 2023

matthewkeil commented Nov 6, 2023

vweevers commented Nov 6, 2023

matthewkeil commented Nov 6, 2023 • edited Loading

vweevers commented Nov 6, 2023

matthewkeil commented Nov 6, 2023

vweevers commented Nov 6, 2023

matthewkeil commented Nov 7, 2023

vweevers commented Nov 7, 2023

matthewkeil commented Nov 8, 2023 • edited Loading

vweevers left a comment

Choose a reason for hiding this comment

matthewkeil commented Nov 20, 2023

ralphtheninja commented Nov 21, 2023

matthewkeil commented Nov 26, 2023

vweevers commented Nov 26, 2023

matthewkeil commented Nov 26, 2023 • edited Loading

vweevers commented Nov 26, 2023 • edited Loading

matthewkeil commented Nov 26, 2023

matthewkeil commented Nov 5, 2023 •

edited

Loading

matthewkeil commented Nov 6, 2023 •

edited

Loading

matthewkeil commented Nov 8, 2023 •

edited

Loading

matthewkeil commented Nov 26, 2023 •

edited

Loading

vweevers commented Nov 26, 2023 •

edited

Loading